Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

health: move to mypy #1620

Merged
merged 14 commits into from
Dec 29, 2024
Merged

health: move to mypy #1620

merged 14 commits into from
Dec 29, 2024

Conversation

WilliamBergamin
Copy link
Contributor

Summary

This PR aims to follow the Bolt Pythons #1130

MyPy was found to execute much fast the PyType in this project, these changes adapt the project to use MyPy instead of PyType

From my short experience with mypy:

  • Good documentation 📜
  • Good error surfacing
  • Easy to configure
  • It offers a language server, this helps with development since it integrate with IDEs

Note: removing all the type: ignore comments from the project may cause breaking changes or have a ripple effect through the entire project

Testing

Category

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.signature (Request Signature Verifier)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.models (UI component builders)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.rtm_v2 (RTM client)
  • /docs (Documents)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)
  • other

Requirements

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

@WilliamBergamin WilliamBergamin added enhancement M-T: A feature request for new functionality area:async labels Dec 20, 2024
@WilliamBergamin WilliamBergamin self-assigned this Dec 20, 2024
@WilliamBergamin WilliamBergamin changed the title Move to my py health: move to mypy Dec 20, 2024
@codecov Codecov
Copy link

codecov bot commented Dec 20, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 81.87809% with 110 lines in your changes missing coverage. Please review.

Project coverage is 85.03%. Comparing base (7b04d01) to head (7ab1fab).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
slack_sdk/web/async_client.py 80.55% 14 Missing ⚠️
slack_sdk/web/client.py 80.55% 14 Missing ⚠️
slack_sdk/web/legacy_client.py 80.55% 14 Missing ⚠️
slack_sdk/socket_mode/builtin/internals.py 47.36% 10 Missing ⚠️
...sdk/oauth/installation_store/amazon_s3/__init__.py 36.36% 7 Missing ⚠️
slack_sdk/audit_logs/v1/logs.py 33.33% 4 Missing ⚠️
slack_sdk/web/internal_utils.py 20.00% 4 Missing ⚠️
slack_sdk/audit_logs/v1/client.py 25.00% 3 Missing ⚠️
...k_sdk/oauth/installation_store/sqlite3/__init__.py 40.00% 3 Missing ⚠️
slack_sdk/socket_mode/aiohttp/__init__.py 66.66% 3 Missing ⚠️
... and 24 more
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1620      +/-   ##
==========================================
- Coverage   85.07%   85.03%   -0.04%     
==========================================
  Files         113      113              
  Lines       12661    12658       -3     
==========================================
- Hits        10771    10764       -7     
- Misses       1890     1894       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WilliamBergamin WilliamBergamin mentioned this pull request Dec 23, 2024
Closed
@seratch seratch added this to the 3.34.1 milestone Dec 23, 2024
seratch
seratch approved these changes Dec 23, 2024
View reviewed changes
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; let one minor comment

slack_sdk/models/dialogs/__init__.py Outdated
placeholder=placeholder,
)
self.min_query_length = min_query_length


class DialogBuilder(JsonObject):
attributes = {} # no attributes because to_dict has unique implementation
attributes = set() # no attributes because to_dict has unique implementation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this property is unused, so changing the type should not affect the behavior. That said, changing from dict to set object may not make sense. This legacy component won't be enhanced anymore but using a set object might be a cause of future bugs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point 💯 it seems like the attribute property is defined as a set in the inherited class (JsonObject), I'm assuming this object overrides this and mypy does not like it. I've reverted the changes and added an ignore comment for now

We can address these ignore comments later or if they become relevant

@WilliamBergamin WilliamBergamin merged commit 0d77f8d into slackapi:main Dec 29, 2024
13 checks passed
@WilliamBergamin WilliamBergamin deleted the move-to-my-py branch December 29, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seratch seratch seratch approved these changes

Assignees

@WilliamBergamin WilliamBergamin

Labels
area:async enhancement M-T: A feature request for new functionality
Projects
None yet
Milestone
3.34.1
Development

Successfully merging this pull request may close these issues.
2 participants
@WilliamBergamin @seratch